-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RLlib] Add a test for loading checkpoint v1.0 Policies using the new Policy.from_checkpoint()
.
#29370
[RLlib] Add a test for loading checkpoint v1.0 Policies using the new Policy.from_checkpoint()
.
#29370
Conversation
) | ||
|
||
policies = Policy.from_checkpoint(path_to_checkpoint) | ||
self.assertTrue("default_policy", policies) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"default_policy" in policies
Also, nit nit: Can we use DEFAULT_POLICY_ID instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. thanks.
|
||
print(algo.train()) | ||
algo.stop() | ||
|
||
def test_v1_policy_from_checkpoint(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my understanding: Any reason, why we shouldn't just do this test case in a loop?
B/c you just want to test Policy (not algo)? Python/pickle versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean merge this test with the one above?
the thing is that I need to test DQN policies here. because that is the one we didn't finish migrating to PolicyV2.
it feels a special case compared to the other backward compatibility tests.
we can also remove this test once I move DQN, etc to PolicyV2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice PR! Thanks for keeping us honest wrt backward compat :)
Just one question and one nit.
Policy.from_checkpoint()
.
2495814
to
9a0e977
Compare
…nt() API. Signed-off-by: Jun Gong <[email protected]>
Signed-off-by: Jun Gong <[email protected]>
Signed-off-by: Jun Gong <[email protected]>
9a0e977
to
038f200
Compare
Signed-off-by: Jun Gong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments @gjoliver . Looks good now! :)
Please make sure all tests pass before merging.
… `Policy.from_checkpoint()`. (ray-project#29370) Signed-off-by: Jun Gong <[email protected]> Signed-off-by: Weichen Xu <[email protected]>
Signed-off-by: Jun Gong [email protected]
Why are these changes needed?
Add a unit test to make sure problem fixed by 75e9722 doesn't happen again.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.